Skip to content

Fix acceptance test when run artifact:all#12975

Merged
andsel merged 2 commits intoelastic:masterfrom
andsel:fix/fail_acceptance_tests_on_CI
Jun 10, 2021
Merged

Fix acceptance test when run artifact:all#12975
andsel merged 2 commits intoelastic:masterfrom
andsel:fix/fail_acceptance_tests_on_CI

Conversation

@andsel
Copy link
Contributor

@andsel andsel commented Jun 9, 2021

Release notes

[rn:skip]

What does this PR do?

This PR avoid to check for existence of jar files in logstash-core/lib/jars to decide is run the process gradlew assemble as a task of the rake artifact:all. The motivation is due to the fact that assemble produces directly or indirectly more files than just those jars, for example it also transitively create the plugin_aliases.yml definitions.
The responsibility of the choice to run or not assemble should stay in Gradle's side, becuase it has more clear vision of the dependencies.

Why is it important/What is the impact to the user?

Doesn't impact directly the user

Checklist

  • [] My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

I wasn't able to reproduce this locally, but only on the remote CI using the branch https://github.com/elastic/logstash/tree/investigation_acceptance_fail

Related issues

Use cases

Screenshots

Logs

This commit avoid to check fo rexistence of jar file sto decide if run or not gradle assemble,
basically because the output of assemble task is not only jars but also others, for example plugin-aliases.yml.
In this way the decision to execute or not is left the Gradle logic.
@andsel
Copy link
Contributor Author

andsel commented Jun 9, 2021

Jenkins test this please

@andsel andsel marked this pull request as ready for review June 9, 2021 15:43
Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor suggestion on removing a custom method
... just a FYI - feel free to ignore and merge as is

safe_system("./gradlew", "assemble")
end
puts("Building logstash-core using gradle")
safe_system("./gradlew", "assemble")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi: there's a rake helper sh which does raise on non-0 exit + will print the command run as well as output

Suggested change
safe_system("./gradlew", "assemble")
sh('./gradlew assemble')

safe_system is simply reinventing the wheel here

@andsel
Copy link
Contributor Author

andsel commented Jun 10, 2021

Jenkins test this please

@andsel andsel merged commit 3eaff36 into elastic:master Jun 10, 2021
@andsel andsel changed the title Fix acceptance test when run artifact:deb Fix acceptance test when run artifact:all Jun 10, 2021
andsel added a commit to andsel/logstash that referenced this pull request Jun 17, 2021
This commit avoid to check for existence of jar files to decide if run or not Gradle assemble,
basically because the outputs of assemble task are not only jars but also others files, for example plugin-aliases.yml.
In this way the decision to execute or not is left the Gradle logic.

(cherry picked from commit 3eaff36)
andsel added a commit that referenced this pull request Jun 17, 2021
This commit avoid to check for existence of jar files to decide if run or not Gradle assemble,
basically because the outputs of assemble task are not only jars but also others files, for example plugin-aliases.yml.
In this way the decision to execute or not is left the Gradle logic.

(cherry picked from commit 3eaff36)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants